Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACL: Allow users to query data for their groups, username, and permissions #4774

Merged
merged 11 commits into from
Feb 14, 2020

Conversation

animesh2049
Copy link
Contributor

@animesh2049 animesh2049 commented Feb 14, 2020

This PR enables users to query their groups, username, permissions. Earlier only guardians could do any of that.


This change is Reviewable

Docs Preview: Dgraph Preview

edgraph/access_ee.go Outdated Show resolved Hide resolved
@pawanrawal pawanrawal changed the title Expose user info ACL: Allow users to query data for their groups Feb 14, 2020
edgraph/access_ee.go Outdated Show resolved Hide resolved
addUserFilter should only work for graphql queries. Also non graphql
query in test to make sure it returns empty result for acl queryies.
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: after the current changes are made and the tests pass

Reviewed 1 of 3 files at r1, 1 of 3 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @animesh2049 and @manishrjain)


edgraph/access_ee.go, line 789 at r1 (raw file):

			addUserFilterToQuery(gq, userId, groupIds)
		}
		for _, pred := range x.AllACLPredicates() {

Add a comment here that the query could have ACL predicates which could be part of the blocked preds and we want to allow access to them, hence we delete them from the blocked preds map.


edgraph/access_ee.go, line 786 at r3 (raw file):

	if len(blockedPreds) != 0 {
		if graphql {

Add a comment saying

For GraphQL requests, we allow filtered access to the ACL predicates. Filter for user_id and group_id is applied for the currently logged in user.


edgraph/access_ee.go, line 833 at r3 (raw file):

// addUserFilterToQuery applies makes sure that a user can access only its own
// acl info by applying filter of userid and groupid to acl predicates

me(func: type(Group))

me(func: type(Group)) @filter(eq(dgraph.xid, ["group1", "group2",..]))

similarly for User.


edgraph/access_ee.go, line 857 at r3 (raw file):

	case "dgraph.user.group":
		newFilter := groupFilter(groupIds)
		gq.Filter = parentFilter(newFilter, gq.Filter)

No need to return the pointer here, the function can just update gq.Filter.


edgraph/access_ee.go, line 860 at r3 (raw file):

	case "~dgraph.user.group":
		newFilter := userFilter(userId)
		gq.Filter = parentFilter(newFilter, gq.Filter)

same here


edgraph/access_ee.go, line 916 at r3 (raw file):

		}
*/
func addUserFilterToFilter(filter *gql.FilterTree, userId string,

We can update filter in place

Op: "AND",
Child: []*gql.FilterTree{filter, newFilter},
}
filter = parentFilter

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ineffectual assignment to filter (from ineffassign)

Reverting this commit because updating pointers inside function
doesn't look a neat way. Method used earlier seems more appropriate.
This reverts commit 5b16b15.
Copy link
Contributor Author

@animesh2049 animesh2049 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 6 files reviewed, 9 unresolved discussions (waiting on @filter, @golangcibot, @manishrjain, and @pawanrawal)


edgraph/access_ee.go, line 789 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment here that the query could have ACL predicates which could be part of the blocked preds and we want to allow access to them, hence we delete them from the blocked preds map.

Done.


edgraph/access_ee.go, line 852 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

							{Value: userId},

Done.


edgraph/access_ee.go, line 849 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

				Args: []gql.Arg{{Value: userId}},

Done.


edgraph/access_ee.go, line 786 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment saying

For GraphQL requests, we allow filtered access to the ACL predicates. Filter for user_id and group_id is applied for the currently logged in user.

Done.


edgraph/access_ee.go, line 833 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

me(func: type(Group))

me(func: type(Group)) @filter(eq(dgraph.xid, ["group1", "group2",..]))

similarly for User.

Done.


edgraph/access_ee.go, line 857 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

No need to return the pointer here, the function can just update gq.Filter.

for that I will have to do something like this

updateFilter(newFilter, &gq.Filter)

func updateFilter(newFilter *gql.Filter, filter **gql.Filter) {
    if **filter == nil {
        *filter = newFilter
...

The current version seems more appropriate to me, so leaving reverted my commit.


edgraph/access_ee.go, line 860 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

same here

Same as above.


edgraph/access_ee.go, line 916 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We can update filter in place

Same as above.


edgraph/access_ee.go, line 886 at r4 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

ineffectual assignment to filter (from ineffassign)

Done.

@animesh2049 animesh2049 merged commit 0e022ca into master Feb 14, 2020
@danielmai danielmai changed the title ACL: Allow users to query data for their groups ACL: Allow users to query data for their groups, username, and permissions Mar 30, 2020
@joshua-goldstein joshua-goldstein deleted the animesh2049/expose-user-info branch August 11, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants